Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

POC - Add tooltip to modal, Modal provider (for computer challenge, language picker, game fork) #2845

Draft
wants to merge 7 commits into
base: devel
Choose a base branch
from
Draft

POC - Add tooltip to modal, Modal provider (for computer challenge, language picker, game fork) #2845

wants to merge 7 commits into from

Conversation

andrewjcasal
Copy link

@andrewjcasal andrewjcasal commented Oct 5, 2024 •
edited
Loading

Fixes #

Screen.Recording.2024-10-05.at.3.44.28.PM.mov

Background

  • We had been unable to add tooltips on modals because modals were never a nested component under the main tree containing the DynamicHelp provider.
  • Feel free to try this out by following the video!

Proposed Changes

  • Infrastructure: add the modal provider as a component under the OGSHelpProvider to consumer tooltip information.
  • ActivateTooltip - React component to make it easy to trigger tooltips anywhere. Happy to update if I'm not handling all cases.
  • Modal Context
  • Modal Provider - A context for modals to be rendered at the top of the tree, right under the DynamicHelp functionality. CURRENTLY used only for creating a computer challenge
  • Modal Registry - This will include the list of modals that will use this logic - currently there's only three. Other modals can be added to the registry over time, or can use the register and unregister functions to do this dynamically.
  • ForkModal and LanguagePickerModal switched to functional components to better utilize useContext.
  • Backwards compatibility with other existing modals.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 5, 2024 •
edited
Loading

Uffizzi Ephemeral Environment Deploying

☁️ https://app.uffizzi.com/github.com/online-go/online-go+com/pull/2845

⚙️ Updating now by workflow run 11506001637.

What is Uffizzi? Learn more!

@anoek
Copy link
Member

anoek commented Oct 12, 2024

I'm switching this to draft until while you work on it, feel free to change it back to ready for merge when you're ready

@anoek anoek marked this pull request as draft October 12, 2024 11:42
@anoek anoek mentioned this pull request Oct 14, 2024
Merged
@andrewjcasal andrewjcasal marked this pull request as ready for review October 24, 2024 19:17
@andrewjcasal andrewjcasal changed the title POC - Add tooltip to modal, Modal provider (only for computer challenge) POC - Add tooltip to modal, Modal provider (for computer challenge, language picker, game fork) Oct 24, 2024
@anoek anoek requested a review from GreenAsJade November 4, 2024 13:43
anoek
anoek reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@anoek anoek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the test message, it seems to function well to me. @GreenAsJade does all of this look good from an RDH perspective?

jest.config.ts
@@ -25,6 +25,9 @@ module.exports = {
"src",
"node_modules"
],
"moduleNameMapper": {
'^@/(.*)': '<rootDir>/src/$1',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the tests!

src/views/HelpFlows/ModalHelp.tsx

export function ModalHelp(): JSX.Element {
return (
<HelpFlow debug={true} id="modal-help" showInitially={true}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the cause of:

image

I think we can probably get rid of that before merging?

GreenAsJade
GreenAsJade suggested changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@GreenAsJade GreenAsJade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the look of Modal implementation now - it cleaned up heaps since last review.

The "test for whether RDH works" is not implemented in the normal RDH way, and it would probably be worth doing it "the RDH way" to totally confirm everything works.

Instead of instantiating a component like ActivateTooltip, you want to get registerTargetItem from the DynamicHelp context and use that to point to the react component that should be annotated. You don't need to "trigger the flow" if you have ShowInitially set, which you do.

This is probably most simply demonstrated in OnlineLeagueSpectatorLanding.tsx and src/views/HelpFlows/OOLSpectatorIntro.tsx .

@GreenAsJade
Copy link
Contributor

@andrewjcasal I realised an even better way of testing this is to get rid of the TBD in GameLog :)

I thought "oh, I will quickly do that right now", but of course that requires GameLogModal to be a new modal, which is not a 2 minute job.

I can do it when I have a moment soonish, or if you add it to the PR, I'll test it out.

GreenAsJade added a commit to GreenAsJade/online-go.com that referenced this pull request Nov 11, 2024
@GreenAsJade
Pull Request online-go#2845: POC - Add tooltip to modal, Modal provid…
16e3b91 
…er (for computer challenge, language picker, game fork)
@GreenAsJade
Copy link
Contributor

See above PR: I made GameLogModal use this, which lets it have RDH about autoscore (🎉). I took out the "test case" RDH, and did a slight refactor as per comment on that commit.

@anoek
Copy link
Member

anoek commented Nov 20, 2024

Marking as draft while stuff is being worked on, mark as ready to merge when everything is set, and thanks again!

@anoek anoek marked this pull request as draft November 20, 2024 12:54
@github-actions GitHub Actions
Copy link

This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the stale Issues or PRs that have been open for a long time with no activity label Dec 21, 2024
@GreenAsJade
Copy link
Contributor

@anoek I wonder if things are stable enough to incorporate this?

@anoek
Copy link
Member

anoek commented Dec 21, 2024

Seemed like it was close from your comments but there's been a lot of silence since then..

@GreenAsJade
Copy link
Contributor

I think it actually was "hey, I've made this all working and nice, what do you all think?" 😝

@GreenAsJade
Copy link
Contributor

I've created at PR for the changes I suggested: #2901

@andrewjcasal might prefer to pull them onto this PR and have this one merged - I'm fine with either way!

@github-actions github-actions bot removed the stale Issues or PRs that have been open for a long time with no activity label Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anoek anoek anoek left review comments

@GreenAsJade GreenAsJade GreenAsJade requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewjcasal @anoek @GreenAsJade